feat(stellar-wallet-snap): add batch proof-of-ownership signing - #267
feat(stellar-wallet-snap): add batch proof-of-ownership signing#267hmalik88 wants to merge 7 commits into
Conversation
|
There was a problem hiding this comment.
Almost LGTM, Great work,
i especially like the array with length style, i agree we should do it if we have a expected length (will try to copy the pattern to other API later)
It is our of scope of this PR, but shall we also update the singleton request of ProofOfOwnerShip SignProofOfOwnershipJsonRpcRequestStruct to use the shared struct?
Left some nit suggestions that may benefit to all non-evm SNAP
and left a question for the permission
| */ | ||
| export const SignProofOfOwnershipBatchSuccessStruct = object({ | ||
| accountId: UuidStruct, | ||
| signature: pattern(string(), /^0x[0-9a-f]{128}$/u), |
There was a problem hiding this comment.
nit:
lets create a utils for 64Byte0xHexStruct?
as SignProofOfOwnershipJsonRpcResponseStruct also share that patten as well
or you can leave it here, we can move to some share utils later
| if (account === undefined) { | ||
| results[index] = { | ||
| accountId, | ||
| error: `Account not found: ${accountId}`, |
There was a problem hiding this comment.
nit:
IMO, we should have a shared message / shared error const or key , as i found that we are doing the same message for other non-evm snap
e.g
results[index] = {
accountId,
error: SignProofOfOwnerShipBatch.AccountNotFound,
}
results[index] = {
accountId,
error: SignProofOfOwnerShipBatch.AccountAddressNotMatch,
}
results[index] = {
accountId,
error: normalizeError(error).message, // custom error
}
...
| const signingRequests: SigningRequest[] = []; | ||
|
|
||
| items.forEach(({ accountId, message }, index) => { | ||
| const account = accountsById.get(accountId.toLowerCase()); |
There was a problem hiding this comment.
nit:
looks like we are using account.entropySource as a sign request key
may be we should enforce this attribute not miss?
but i guess this is fine, as when we derive address, we will cross check if the address is same
| signature: add0x(wallet.signMessage(message, 'hex')), | ||
| }; | ||
| } catch (error) { | ||
| results[index] = { |
There was a problem hiding this comment.
just FYI
sign message throw SignMessageException
we can use it to distinguish the error and use different const error key?
| */ | ||
| KeyringRpcMethod.ListAccountAssets, | ||
| KeyringRpcMethod.ListAccountTransactions, | ||
| ClientRequestMethod.SignProofOfOwnershipBatch, |
There was a problem hiding this comment.
i think we dont need permission for SignProofOfOwnershipBatch ?
as it is a client request method, or there is a special reason?
2 participants
Explanation
This PR adds
signProofOfOwnershipBatchsupport to the Stellar Wallet Snap.The new method lets MetaMask request proof-of-ownership signatures for multiple Stellar accounts in one Snap RPC call. Results preserve input order and return per-item success/error objects, so one invalid account or message does not fail the entire batch.
References
N/A
Checklist